ID:2772166
 
BYOND Version:514
Operating System:Windows 10 Pro 64-bit
Web Browser:Chrome 98.0.4758.102
Applies to:DM Language
Status: Open

Issue hasn't been assigned a status value.
Descriptive Problem Summary:

Server is running 514.1575

This is going to be one of those weird issues where its incredibly difficult to reproduce, but we have managed to have world.Export() calls send the results to the wrong callback.

In Paradise, we have 2. One in client/New() for checking when BYOND accounts were made, and one for looking up IPIntel. The IPIntel call to world.Export() managed to end up with BYOND profile data.

Runtime stacktrace: https://i.imgur.com/B2Wkorj.png

The two separate world.Export() calls are https://github.com/ParadiseSS13/Paradise/blob/master/code/ modules/client/client_procs.dm#L986 and https://github.com/ParadiseSS13/Paradise/blob/master/code/ controllers/subsystem/ipintel.dm#L142

Numbered Steps to Reproduce Problem:

I have no idea how you would even start to reproduce this, or how this even happens. Scuffed threading I guess?

Expected Results:

world.Export() calls to not mix results.

Actual Results:

They mixed by some unholy miracle.

Does the problem occur:
Every time? Or how often? This is the first time I have seen this
In other games? N/A
In other user accounts? N/A
On other computers? N/A

When does the problem NOT occur?

99% of the time

Did the problem NOT occur in any earlier versions? If so, what was the last version that worked? (Visit http://www.byond.com/download/build to download old versions for testing.) I have no idea if this appeared in earlier versions given how spurious it is.

Workarounds:

Use an external library for HTTP calls.
At a glance I'm not seeing how this is even possible, since the ID for resuming the background proc is included in the call. But I'm looking into it.

However, although this doesn't necessarily relate to your issue (then again I wouldn't rule it out) you should not be doing this kind of thing in client/New(). As a general rule client/New() should be limited to code that doesn't involve external queries or anything that will have to sleep.
In response to Lummox JR
Lummox JR wrote:
However, although this doesn't necessarily relate to your issue (then again I wouldn't rule it out) you should not be doing this kind of thing in client/New(). As a general rule client/New() should be limited to code that doesn't involve external queries or anything that will have to sleep.

I should have pointed out that this check is inside a proc with `set waitfor = FALSE`, so it isnt blocking.
After looking at the code I'm definitely stumped. This is the process that happens on the backend when calling world.Export() for an HTTP request:

1) world.Export() gets a new "pid" value for a background proc ID, and calls TransmitSrv2SrvMsg().
2) TransmitSrv2SrvMsg() creates a structure that handles the HTTP request, and passes the pid to that, then returns a true value indicating to the caller that it should sleep the proc.
3) The caller duplicates the proc for sleeping and sets up the appropriate background proc record, then halts.
4) At some point in the future, the HTTP request succeeds or fails. (Remember this code is not threaded; this is all one thread.) A return value is generated and then sent to TransmissionReturn() with the pid value.
5) TransmissionReturn() looks up the waiting proc by its pid and sets the return value, adding the proc to the scheduler so it will resume as soon as the engine is ready.

Based on what I'm seeing so far I don't see how it's at all possible for this to happen, unless something really weird is happening with the background proc ID numbers. I'm continuing to look, though.